-
Notifications
You must be signed in to change notification settings - Fork 258
[ETHEREUM-CONTRACTS] Migrate to OpenZeppelin v5 #2097
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
Signed-off-by: pavedroad <[email protected]>
We could change EVM version to |
packages/automation-contracts/scheduler/contracts/VestingSchedulerV3.sol
Show resolved
Hide resolved
packages/automation-contracts/scheduler/contracts/VestingSchedulerV2.sol
Show resolved
Hide resolved
|
||
console.log("Revert with overflow."); | ||
vm.expectRevert("SafeCast: value doesn't fit in 96 bits"); | ||
vm.expectRevert(); // SafeCastOverflowedIntDowncast |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is no reference to it?
|
||
console.log("Revert with overflow."); | ||
vm.expectRevert("SafeCast: value doesn't fit in 96 bits"); | ||
vm.expectRevert(); // SafeCastOverflowedIntDowncast |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
newCtx = ctx; | ||
|
||
if (pool.superToken().isPool(this, memberAddr)) { | ||
if (memberAddr == address(0) || pool.superToken().isPool(this, memberAddr)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sneaky.
error NO_PROXY_LOOP(); // 0z66750bca | ||
|
||
constructor(address implementation_) UpgradeableBeacon(implementation_) {} | ||
constructor(address implementation_) UpgradeableBeacon(implementation_, _msgSender()) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note:
- Context._msgSender
- Ownable is Context
() => superfluid.getSimpleACL(), | ||
"SIMPLE_ACL" | ||
); | ||
// SimpleACL has now been deployed on all networks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. What about deploying to a new network, does the bootstrap logic still exist?
vm.stopPrank(); | ||
|
||
// cannot connect the zero address | ||
vm.startPrank(alice); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay
spender, | ||
_allowances[holder][spender].sub(amount, "SuperToken: transfer amount exceeds allowance")); | ||
require(amount <= _allowances[holder][spender], "SuperToken: transfer amount exceeds allowance"); | ||
// TODO: this triggers an `Approval` event, which shouldn't happen for transfers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't recall when did this creep in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a draft to https://github.com/orgs/superfluid-org/projects/1/views/1 for later to address.
// Go to https://hardhat.org/config/ to learn more | ||
|
||
// Remapping for OpenZeppelin contracts | ||
subtask(TASK_COMPILE_GET_REMAPPINGS).setAction( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider to drop a section in the https://github.com/superfluid-org/protocol-monorepo/blob/dev/packages/ethereum-contracts/README.md
* | ||
* CAUTION: This file is deprecated as of v4.9 and will be removed in the next major release. | ||
*/ | ||
contract ERC1820Implementer is IERC1820Implementer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider to move it under contracts/utils/
What & Why
see #2016
How
Imports and notes about what changed:
openzeppelin-contracts is now a git submodule (foundry style) instead of an npm package.
In order to stay compatible with hardhat (used by legacy tests) and truffle (used by ops scripts), an npm postinstall script places a symlink to that submodule in node_modules. This was found to be the only solution which doesn't cause significant issues with either hardhat or truffle (see dedicated comment).
The openzeppelin imports in the ethereum-contracts package are now qualified by the major version, that is
@openzeppelin-v5/...
. This allows contracts using SF contracts as a dependency to use the default namespace@openzeppelin
with the OZ version they prefer. The OZ package is now declared a peerDependency.Hardhat projects need to set up a remapping task in their config file, see CHANGELOG.